fix(iac): refresh-outputs merges live Outputs (preserve fields not in Read response)#575
Merged
Conversation
…fields Replace semantics in refreshOne clobbered create-time fields absent from the cloud Read response (e.g., DO Droplet user_data). After refresh, state held user_data="" causing planner to emit a REPLACE, which hit a 422 when the Volume was already attached (TC2 run 25508442022). New merge semantics: start from src.Outputs clone, overlay live.Outputs keys, so fields not returned by Read are preserved while cloud truth wins for fields that are returned. Adds three new tests (MergePreservesFieldsNotInRead, LiveOverridesExisting, NewFieldsFromLiveAdded) and ADR 011. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR changes iac/refreshoutputs to avoid clobbering persisted output fields that are not returned by a provider’s Read endpoint (e.g., write-only/create-time fields like DO Droplet user_data). It updates refresh behavior from “replace Outputs” to “merge Outputs”, adds regression tests for the intended merge semantics, and documents the decision in an ADR.
Changes:
- Update
refreshOneto mergelive.Outputsinto a clone ofsrc.Outputs, preserving fields absent from Read. - Add tests covering: preserve absent fields, live overrides stale values, and new live fields get added.
- Add ADR 011 documenting merge semantics and trade-offs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
iac/refreshoutputs/refresh.go |
Implements merge semantics when reconciling dst.Outputs from provider Read results. |
iac/refreshoutputs/refresh_test.go |
Adds tests validating merge behavior across preserve/override/add scenarios. |
docs/adr/011-refresh-outputs-merge-semantics.md |
Documents rationale, decision, and trade-offs for merge semantics. |
| needsUpdate := false | ||
| merged := cloneMap(src.Outputs) | ||
| for k, v := range live.Outputs { | ||
| if existing, ok := merged[k]; !ok || !reflect.DeepEqual(existing, v) { |
Comment on lines
+115
to
+119
| merged := cloneMap(src.Outputs) | ||
| for k, v := range live.Outputs { | ||
| if existing, ok := merged[k]; !ok || !reflect.DeepEqual(existing, v) { | ||
| merged[k] = v | ||
| needsUpdate = true |
Comment on lines
+221
to
+236
| func TestRefresh_MergePreservesFieldsNotInRead(t *testing.T) { | ||
| states := []interfaces.ResourceState{ | ||
| { | ||
| Name: "coredump-staging-droplet", | ||
| Type: "infra.droplet", | ||
| ProviderID: "droplet-1", | ||
| // user_data was captured at create-time; Read won't return it. | ||
| Outputs: map[string]any{"id": "x", "user_data": "<script>init</script>"}, | ||
| }, | ||
| } | ||
| fakeProvider := &fakeIaCProvider{readOutputs: map[string]map[string]any{ | ||
| // provider Read only returns id — user_data is omitted (write-only on Read) | ||
| "droplet-1": {"id": "x"}, | ||
| }} | ||
|
|
||
| refreshed, err := Refresh(context.Background(), fakeProvider, states, Options{Concurrency: 1}) |
Address Copilot review findings: - nil src.Outputs + non-empty live.Outputs panicked on map assignment. Switch to copy-on-write: allocate the merged map only on first detected difference, seeding it with maps.Copy(merged, src.Outputs) which is nil-safe (copies nothing from nil). - Remove per-resource unconditional clone (now only allocated when a change is detected). - Remove the now-unused cloneMap helper. - Add TestRefresh_NilSrcOutputs_LiveFieldsPopulated (would have caught the nil-map panic). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
⏱ Benchmark Results✅ No significant performance regressions detected. benchstat comparison (baseline → PR)
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
refreshOnereplaceddst.Outputsentirely withlive.Outputsfrom the cloud Read response. Cloud Read endpoints that don't return all create-time fields (e.g., DO Dropletuser_datais write-only on Read) caused those fields to be silently zeroed in state.state.user_data=""vsconfig.user_data="<cloud-init>"→ emitted REPLACE. Apply tried delete+create; Volume already attached → 422. This broke TC2 cutover run 25508442022.refreshOnestarts from a clone ofsrc.Outputs, overlays alllive.Outputskeys (cloud truth wins for returned fields), and preserves fields absent from the Read response.Changes
iac/refreshoutputs/refresh.go— merge logic replaces the single-line replace inrefreshOneiac/refreshoutputs/refresh_test.go— three new tests:TestRefresh_MergePreservesFieldsNotInRead—user_dataabsent from Read response is preservedTestRefresh_LiveOverridesExisting— cloud-returned value overrides stale state valueTestRefresh_NewFieldsFromLiveAdded— new fields from cloud Read are added to statedocs/adr/011-refresh-outputs-merge-semantics.md— records context, decision, and trade-offsTrade-offs (per ADR 011)
If a cloud provider truly removes a field from a resource's outputs, refresh-outputs keeps the stale value. This is acceptable: cloud removals of IaC-managed output fields are rare, and the remediation (
wfctl infra apply --refresh) is well-defined. The alternative (replace semantics) causes false REPLACE storms for write-only fields — a far more disruptive failure mode.Test plan
go test ./iac/refreshoutputs/...— all 8 tests passgo vet ./iac/refreshoutputs/...— cleanReferences
user_datais create-time-only, not returned by Read🤖 Generated with Claude Code